Skip to content

Conversation

@rachelmcr
Copy link
Contributor

@rachelmcr rachelmcr commented Nov 23, 2022

Closes: #8190

Description

This PR adds a set of fake OrderStatsv4 data we can use to configure the Revenue and Order Analytics cards until real data is integrated into the Analytics Hub (in #8189).

Submitter Checklist

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@rachelmcr rachelmcr added the category: tracks Related to analytics, including Tracks Events. label Nov 23, 2022
@rachelmcr rachelmcr added this to the 11.4 milestone Nov 23, 2022
@rachelmcr rachelmcr marked this pull request as ready for review November 23, 2022 13:44
@peril-woocommerce
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

Copy link
Contributor

@Ecarrion Ecarrion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good but I fail to see the benefit of this fake data, is it to test the helpers? 🤔


/// Order stats for the current selected time period
///
@Published private var currentOrderStats: OrderStatsV4 = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for these to be published properties? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought if we make them published properties, we can listen for changes to the data (i.e. when a different time period is selected and new data is synced) to update what is displayed on the cards. WDYT?

@rachelmcr
Copy link
Contributor Author

@Ecarrion I thought it would be useful so we can split the work of integrating the Yosemite layer (retrieving real OrderStatsV4 data for the current and previous periods) and using OrderStatsV4 data to configure the Orders and Revenue cards (which can be done with the fake data for now). Related discussion: p1669207327936629/1669206497.378969-slack-C02KUCFCSFP

Is there a better way to split up that work so we can do it in parallel? I'm open to anything; this just seemed like a simple solution.

@rachelmcr
Copy link
Contributor Author

It just occurred to me that we don't need all the fake data to get started with this if we just set the order data properties to nil:

    /// Order stats for the current selected time period
    ///
    @Published private var currentOrderStats: OrderStatsV4? = nil

    /// Order stats for the previous time period (for comparison)
    ///
    @Published private var previousOrderStats: OrderStatsV4? = nil

This is simpler and should still be appropriate as a starting point for splitting up the work (we won't have any order data until we perform the remote sync, so the view model should be initialized with nil order data in any case).

@Ecarrion
Copy link
Contributor

Hmmm, I don't know 😓

I was thinking that the "how we store the data" was going to depend on how we fetch it. For example, on my mind, we would have done something like

rangeTrigger()
   .fetchStatsData()
   .bindToCardsVMs()

So if we go with that route, we don't need to store the stats data response, does that make sense?
For that reason, I normally wouldn't get ahead of myself on how I store the data here. Am I missing something?

In any case, we can always delete this if it doesn't serve our purpose, so feel free to merge it as it wouldn't cause much harm, I hope 😛

@Ecarrion Ecarrion assigned Ecarrion and unassigned Ecarrion Nov 23, 2022
@rachelmcr
Copy link
Contributor Author

Ah, gotcha. Because the stats store doesn't return the stats response directly (only upserts it into storage) I figured we'd store the data in the view model and do the fetching separately from the binding to card VMs:

rangeTrigger()
   .fetchStatsData()
   .updateDataPropsFromStorage()

observeDataProps()
   .maptoCardVMs()

But as you said we can delete this later if it doesn't serve our purpose, and any work that we've done in parallel could be stitched together if we decide not to store the data like this. I'll make the change I mentioned earlier and then merge this.

@rachelmcr rachelmcr enabled auto-merge November 23, 2022 16:25
@wpmobilebot
Copy link
Collaborator

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8191-5bfcbed on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@rachelmcr rachelmcr merged commit a3c0df6 into trunk Nov 24, 2022
@rachelmcr rachelmcr deleted the issue/8190-fake-orders-data branch November 24, 2022 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: tracks Related to analytics, including Tracks Events.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Analytics Hub] Add fake OrderStatsV4 data for configuring Analytics cards

4 participants